Repository navigation
Conversation
zakahan
left a comment
zakahan
left a comment
There was a problem hiding this comment.
This is by an automated agent, not a human.
Reviewed head 949c2e3 against the merge base with main. The snapshot/retention design is coherent and the new backend tests cover the main flows, but the PR is currently CONFLICTING/DIRTY against main: the recorded CI is from the Aug 6 base, so a rebase and a fresh CI run are required before this can be considered mergeable. Findings are inline.
Verification
uv run --extra dev pytest tests/cli/test_frontend_sandbox.py -q -k "snapshot or retention or capabilities"at the reviewed head: 9 passed.gh pr checksat the head commit: green (test 3.10/3.12, pre-commit, gitleaks, verify), but the runs predate the current base; merge state isCONFLICTING.- Not run:
npm --prefix frontend test/npm --prefix frontend run build, and theAGENTS.md-required real-browser checks for the new launch-dialog states (capability loading/error, retention choice) and the wakeable-agent flow. The PR description reports npm tests/build, but the new UI states and narrow-window/keyboard paths are not browser-verified in the change evidence.
| owner_prefix | ||
| ): | ||
| return None | ||
| return user_session_id |
There was a problem hiding this comment.
[Minor] For the new studio-<owner>-<uuid> UserSessionId format this returns the raw UserSessionId, so _public_snapshot and the My Agents card title show studio-<owner>-<uuid> for every wakeable agent, while legacy studio2-* snapshots decode their original name (the new test asserts this fallback). If that is not intentional, encode the agent display name in the UserSessionId the way the legacy scheme did, or read the snapshot's display name when the API exposes one.
| # shipped as a plain env var. | ||
| os.environ["IAM_ROLE"] = role_trn | ||
|
|
||
| # A single legacy Tool was snapshot-enabled. Preserve it as the recoverable |
There was a problem hiding this comment.
[Minor] This migration reclassifies any explicitly supplied --sandbox-chat-codex-tool-id (same for OpenClaw/Hermes) without a snapshot id as the legacy recoverable Tool and then provisions a new temporary Tool. The option help describes the flag as the temporary-chat Tool, so a later deploy that pins the temporary Tool through it silently turns that Tool into the recoverable one. Consider an explicit migration switch, or at least a warning when an explicitly supplied id is reclassified.
| return session | ||
| return None | ||
|
|
||
| async def resume_snapshot( |
There was a problem hiding this comment.
[Minor] resume_snapshot does list -> _active_session_for_snapshot -> ResumeSessionFromSnapshot with awaits between the check and the act and no per-snapshot lock. Two concurrent wake requests (two tabs, or owner plus admin) can both observe no active Session and resume the same snapshot twice. Serialize per (tool_id, snapshot_id) with an asyncio.Lock, or make the resume idempotent by snapshot id.
Summary
UserSessionIdvalues, while preserving administrator visibility and legacy snapshot compatibilityCreateToolandUpdateToolwrites to approximately three requests per second during Studio deploymentValidation
npm test— 411 passednpm run builduv run pytest tests/cli/test_frontend_sandbox.py tests/cli/test_frontend_sandbox_options.py tests/cli/test_frontend_skill_creator.py tests/cli/test_studio_deploy_target.py tests/cli/test_studio_sandbox_tools.py tests/cli/test_studio_update.py -q— 121 passeduv run pre-commit run --all-files